-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
xds: use google default creds #3673
Conversation
So the client works not only on GCE (e.g. it also read env variable for creds).
credentials/google/google.go
Outdated
@@ -31,7 +31,7 @@ import ( | |||
"google.golang.org/grpc/internal" | |||
) | |||
|
|||
const tokenRequestTimeout = 30 * time.Second | |||
const tokenRequestTimeout = 3 * time.Second |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm .. what leak is this?
b88ebbd
to
4f9aee7
Compare
The leak is caused by I'm going to update leadcheck to ignore this goroutine. |
…d disable leakcheck" This reverts commit f3ffd29.
…nly disabling part of it doesnt work, because another test still see the leak
Check scope and do different things.
This PR now also changes gRPC's google default creds to skip oauth if scope is not set. Please take a look again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could someone who understand this whole "application default creds" business better also take a look. Thanks.
@@ -58,19 +56,11 @@ var ( | |||
} | |||
) | |||
|
|||
type s struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we please add a TODO here to re-add the leak check once the leak is fixed upstream.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I asked @ejona86 to take a look at the creds changes. And also tested locally and on GCE.
@@ -58,19 +56,11 @@ var ( | |||
} | |||
) | |||
|
|||
type s struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
98d25c7
to
de690bc
Compare
It panics on appgine with go1.9
And I looked at them at they seemed sane. You left off the surprising part! (That I actually, promptly, looked at it and replied.) |
de690bc
to
e16fab6
Compare
@@ -1,3 +1,5 @@ | |||
// +build !appengine |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test is disabled for appengine, because of https://travis-ci.org/github/grpc/grpc-go/jobs/697000659#L525
use google default creds, so the client works not only on GCE (e.g. it also read env variable for creds).
Change google default creds to use jwt directly if scope is not set.
Leak check is disabled temporarily due to compute/metadata: goroutine leak googleapis/google-cloud-go#2417